feat(notebook-migration-service): compute jupyter iframe url per request - #7602
Conversation
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7602 +/- ##
============================================
- Coverage 89.62% 89.39% -0.24%
+ Complexity 4395 4375 -20
============================================
Files 1177 1177
Lines 46884 46835 -49
Branches 5239 5225 -14
============================================
- Hits 42022 41868 -154
- Misses 3109 3220 +111
+ Partials 1753 1747 -6
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/request-review @mengw15 |
There was a problem hiding this comment.
Pull request overview
Makes notebook iframe URL generation stateless by deriving it from each request instead of shared process state.
Changes:
- Removes the process-global iframe URL state.
- Adds optional, validated notebook-name selection with a backward-compatible default.
- Adds tests for default, explicit, invalid, and stateless URL generation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
NotebookMigrationResource.scala |
Computes validated iframe URLs per request and removes shared state. |
NotebookMigrationResourceSpec.scala |
Tests explicit, invalid, default, and stateless iframe URL behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
mengw15
left a comment
There was a problem hiding this comment.
Three comments. Copilot's three all hold up as well — the spec one especially: the positive test goes through the companion object, so the new query-param plumbing is the one part with no coverage.
…g via the resource class
…ce in setNotebook comment
…rl/token constraint
I have raised issue #7671 to keep track of the frontend change |
apache#7738) ### What changes were proposed in this PR? Uploads each workflow's notebook to Jupyter under a per-workflow filename instead of a single shared `notebook.ipynb`. Before this change the frontend always uploaded to `work/notebook.ipynb`. That was safe across users (each runs their own pod) but not across one user's workflows: every workflow wrote to the same file, so opening a second workflow overwrote the first, and because nothing writes back from Jupyter, any edits made in the panel were lost. Two tabs on different workflows also collided on the same file. This PR keys the notebook file on the workflow id (`notebook_<wid>.ipynb`) so each workflow has its own. The backend already accepts this (from apache#7602): `get-jupyter-iframe-url` takes an optional `notebookName` query param and `set-notebook` accepts any `[A-Za-z0-9._-]+\.ipynb` name, which `notebook_<wid>.ipynb` satisfies. No backend change is needed. **`NotebookMigrationService`** - Adds an exported `notebookFileName(wid)` helper (mirrors the existing `notebookMappingKey`) that returns `notebook_<wid>.ipynb`, or the default when there is no wid. - `sendNotebookToJupyter(notebookData, notebookName)` takes the name instead of hardcoding it. - `getJupyterIframeURL(notebookName?)` sends the name as the `notebookName` query param when given, and omits it otherwise so the backend default still applies. **`JupyterPanelService` (owns the name)** - Adds a private `currentNotebookFileName()` that derives the filename from the current workflow's wid, and uses it for both the upload and the iframe fetch so the two can never derive different names. - Adds a public `getJupyterIframeURLForWorkflow()` that the panel calls to get the URL for the current workflow's notebook. **`JupyterNotebookPanelComponent` (view)** - Calls `jupyterPanelService.getJupyterIframeURLForWorkflow()` and drops its now-unused direct dependency on `NotebookMigrationService`. Because the upload and the iframe fetch both go through `currentNotebookFileName()`, the file that is written and the file the panel requests are always the same, and switching workflows produces a distinct `notebook_<wid>.ipynb` rather than overwriting a shared one. ### Any related issues, documentation, discussions? Closes apache#7671 Parent issue apache#4301 Follow-up: deleting a notebook now leaves its `notebook_<wid>.ipynb` file in the Jupyter pod, since `deleteNotebookAndMapping` only removes the database rows. This was self-limiting under the old single-file scheme. Tracked in apache#7737 ### How was this PR tested? - `notebook-migration.service.spec.ts`: `notebookFileName` mapping, the request body carrying the name on `sendNotebookToJupyter`, and the `notebookName` query param being present when a name is given and absent when it is not. - `jupyter-panel.service.spec.ts`: the upload uses the wid-derived filename, `getJupyterIframeURLForWorkflow` forwards that same filename to the HTTP client, and the disabled-flag path returns null without any HTTP call. - `jupyter-notebook-panel.component.spec.ts`: the panel fetches its URL through `getJupyterIframeURLForWorkflow`. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 4.8)
What changes were proposed in this PR?
Makes
notebook-migration-servicestateless so it can later run as a single global instance instead of one instance per user. This is the first backend stage of moving the service onto Texera's "orchestrator services are global, stateful resources are per user" pattern.Today the service keeps a shared
@volatile jupyterIframeURL:set-notebookwrites it andget-jupyter-iframe-urlreads it back. That shared state is only safe because each user happens to run their own pod, and even within one user it lets two browser tabs race. This PR removes the shared state and builds the URL from the request instead.NotebookMigrationResource.scala@volatile var jupyterIframeURLsingleton and the warning comment that documented its per-user-pod assumption. Adds adefaultNotebookNameconstant (notebook.ipynb).getJupyterIframeURLnow takes anotebookNameargument and builds the URL on each call. The name is validated with the same plain.ipynbregexsetNotebookuses, since it now flows straight into the returned URL (blocks path traversal). The argument defaults todefaultNotebookName.setNotebookno longer mutates any shared state; the assignment that wrote the singleton is gone. Its upload behavior is unchanged./get-jupyter-iframe-urlendpoint accepts an optionalnotebookNamequery parameter and falls back to the default when it is absent.The change is backward compatible. The existing frontend calls the endpoint with no query parameter, which resolves to
notebook.ipynb, exactly the URL it received before. No frontend, config, or deployment change is needed in this PR, and no other service or branch consumes the removed state.Any related issues, documentation, discussions?
Closes #7390
Parent-issue #4301
How was this PR tested?
Extends the existing suite in
NotebookMigrationResourceSpec.scala:notebookNameis honored in the returned URL.notebookNameis rejected with 400 before any Jupyter call.setNotebookuploadsother.ipynb, a parameter-lessgetJupyterIframeURLreturns the defaultnotebook.ipynb, proving the result no longer depends on state left bysetNotebook.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.8)